Skip to content

Believe forwarded client addresses only from trusted proxies - #16

Merged
dabeckham merged 2 commits into
mainfrom
fix/trusted-proxy-client-ip
Jul 26, 2026
Merged

Believe forwarded client addresses only from trusted proxies#16
dabeckham merged 2 commits into
mainfrom
fix/trusted-proxy-client-ip

Conversation

@dabeckham

Copy link
Copy Markdown
Owner

The problem

The anonymous rate limit and the upload audit trail are both keyed on the client
address. An address the client can choose is an address that resets the budget
and writes fiction into the audit trail.

Two layers were each trusting X-Forwarded-For on their own:

  1. app/fingerprint.py read the header whenever TRUST_FORWARDED_FOR was set,
    and took split(",")[0] — the client-controlled end of the chain, since
    proxies append.
  2. The container ran uvicorn with --proxy-headers --forwarded-allow-ips "*",
    which rewrote the peer address from the same header for any caller,
    before the application saw it.

Consequence of (2): TRUST_FORWARDED_FOR=false was not an off switch. With
it set to false, a forged header still landed in the audit trail, because the
server had already rewritten the peer address.

Is it reachable?

Tested rather than assumed.

  • From the internet: no. The app publishes no host port, and the front
    replaces X-Forwarded-For on the way through. A forged header sent to
    https://share.beckham.ai/api/anon-upload was discarded; the row recorded the
    real address.
  • From the container network: yes. Four other containers share idp_proxy
    with this app. A POST straight to beckham-share-app:8000 carrying
    X-Forwarded-For: 198.51.100.7 was recorded verbatim in upload_events.

So the exposure is "any container on the shared proxy network", not "anyone on
the internet" — but that is exactly the layer the in-app control exists to
backstop.

The change

uvicorn now reports the socket peer, and the application makes the decision in
one place:

  • New TRUSTED_PROXIES setting — comma-separated addresses, CIDR ranges, or
    hostnames, resolved at runtime because Docker assigns container addresses.
    Defaults to idp-caddy, which resolves to 172.23.0.5 from inside the app
    container.
  • Forwarded headers are honoured only when the request arrived from one of
    those peers. Everything else is attributed to the address it came from,
    whatever headers it carries.
  • The chain is read right to left: trusted hops are skipped, and the first
    untrusted address is the closest one a trusted proxy actually observed.
  • TRUST_FORWARDED_FOR is replaced by TRUSTED_PROXIES. Nothing set the old
    variable, so there is nothing to migrate.

What is deliberately not changed

The browser fingerprint stays client-supplied. It is OR-ed with the address,
not substituted for it — app/ratelimit.py counts rows matching either key,
so rotating the fingerprint cannot buy a fresh budget while the address key
still matches. Its job is the reverse: catching one device that rotates
addresses. Folding the address into it would destroy exactly that property.
Now stated explicitly in docs/SECURITY.md so the distinction reads as a
decision rather than an oversight.

Verification

Staging container, real requests:

Case Result
Untrusted neighbour forges the header Recorded 172.23.0.8 — the address it came from
Same request, that range trusted Recorded 198.51.100.7 — trusted forwarding still works
Chain 198.51.100.7, 203.0.113.9 from a trusted peer Recorded 203.0.113.9 — client-appended hop discarded

Plus 8 unit tests over client_ip() and a regression test that rotating the
header does not reset the anonymous budget. Backend suite: 33 tests pass.

Deploy note

This changes the container command, so it needs a rebuild rather than a restart.
docs/SECURITY.md §3 and §6 and docs/CONFIGURATION.md are updated to match.

The anonymous rate limit and the upload audit trail are both keyed on the
client address, so an address the client can choose is an address that resets
the budget and writes fiction into the audit trail.

Two layers were each trusting the header on their own:

- app/fingerprint.py read X-Forwarded-For whenever TRUST_FORWARDED_FOR was
  set, and took split(",")[0] — the client-controlled end of the chain, since
  proxies append.
- the container ran uvicorn with --proxy-headers --forwarded-allow-ips "*",
  which rewrote the peer address from the same header for any caller, before
  the application saw it. TRUST_FORWARDED_FOR=false did not prevent this; the
  documented off switch was not an off switch.

The app is not published to a host port, so this is not reachable from the
internet — the front replaces the header on the way through. It is reachable
from the four other containers that share the idp_proxy network: a POST
straight to beckham-share-app:8000 with a forged X-Forwarded-For was recorded
verbatim in upload_events.

Now uvicorn reports the socket peer and the application makes the decision in
one place. Forwarded headers are honoured only when the request arrived from a
peer in the new TRUSTED_PROXIES setting (addresses, CIDR ranges, or hostnames
resolved at runtime, since Docker assigns container addresses), and the chain
is read right to left so hops appended by the client are discarded.
TRUST_FORWARDED_FOR is replaced by TRUSTED_PROXIES; nothing set it.

The browser fingerprint is deliberately left as it is. It is client-supplied
by nature, and it is OR-ed with the address rather than substituted for it, so
rotating it cannot buy a fresh budget while the address key still matches. Its
job is to catch one device rotating addresses. Documented in docs/SECURITY.md
so the distinction is not mistaken for an oversight.

Verified on a staging container: an untrusted neighbour's forged header is now
ignored and the request is attributed to the address it came from; a trusted
peer's header is still honoured; a chain of "forged, real" resolves to the
real hop. Eight unit tests cover the same cases.
@dabeckham
dabeckham merged commit 9705d00 into main Jul 26, 2026
2 checks passed
@dabeckham
dabeckham deleted the fix/trusted-proxy-client-ip branch July 26, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant